Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept Both Functions and Components for DayPickerInput #862

Closed
wants to merge 3 commits into from

Conversation

glockjt
Copy link

@glockjt glockjt commented Feb 5, 2019

Problem:
React was throwing an error when a function was passed into the component property for the DayPickerInput

<DayPickerInput component={props => <input {...props} />} />

This address #748

Explanation:
The docs example for component shows the above code. However it is using it and expecting a React Component, thus resulting in React throwing the following error:

image

Resolution:
Add a check to determine if the component is a function or not and if it is a function render the function by passing all inputProps. If the component is a react component render as a component.

@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #862 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #862   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645
Impacted Files Coverage Δ
src/DayPickerInput.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ad48e...1db3187. Read the comment docs.

3 similar comments
@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #862 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #862   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645
Impacted Files Coverage Δ
src/DayPickerInput.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ad48e...1db3187. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #862 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #862   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645
Impacted Files Coverage Δ
src/DayPickerInput.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ad48e...1db3187. Read the comment docs.

@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #862 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #862   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         645    645           
  Branches      141    141           
=====================================
  Hits          645    645
Impacted Files Coverage Δ
src/DayPickerInput.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ad48e...1db3187. Read the comment docs.

onKeyUp: this.handleInputKeyUp,
onClick: !inputProps.disabled ? this.handleInputClick : undefined,
})
)}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly recommend to extract common props to the const.

const props = {
            placeholder: this.props.placeholder,
            ...inputProps,
            value: this.state.typedValue || this.state.value,
            onChange: this.handleInputChange,
            onFocus: this.handleInputFocus,
            onBlur: this.handleInputBlur,
            onKeyDown: this.handleInputKeyDown,
            onKeyUp: this.handleInputKeyUp,
            onClick: !inputProps.disabled ? this.handleInputClick : undefined,
};

and then spread it to child.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extracted.

onClick={!inputProps.disabled ? this.handleInputClick : undefined}
/>
{this.props.component &&
{}.toString.call(this.props.component) !== '[object Function]' ? (

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just typeof this.props.component === 'function'?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way is more verbose. Not really relevant here but one good example is that it will return Array for Array instead of Object. A better way would be to actually have a function that would extract out Function.

@codecov-io
Copy link

codecov-io commented Apr 30, 2019

Codecov Report

Merging #862 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #862   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines         659    659           
  Branches      146    146           
=====================================
  Hits          659    659
Impacted Files Coverage Δ
src/DayPickerInput.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 421af9f...72a0683. Read the comment docs.

@saintplay
Copy link

Any progress on merging this?

@gpbl
Copy link
Owner

gpbl commented Oct 18, 2019

This will be solved in the upcoming v8: see #942 . Thanks!

@gpbl gpbl closed this Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants